Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wp_remote_get now sends the site url as referer #13664

Merged
merged 4 commits into from
Oct 14, 2019
Merged

wp_remote_get now sends the site url as referer #13664

merged 4 commits into from
Oct 14, 2019

Conversation

wigglemuff
Copy link
Contributor

Changes proposed in this Pull Request:

Testing instructions (Before the fix):

  • Go to widgets page and add Contact Info & Map widget by Jetpack
  • Enter Google's API key (without HTTP referrer restrictions) as explained in Jetpack docs. Embed works and we can see a preview after saving the widget
  • Remove the API key, save the widget (basically reset the widget and start fresh)
  • Go to Google API page and enable HTTP referrer restrictions to your site URL. Now on the widgets page, enter Google's API key again and save the widget. It returns an error Google Maps Platform rejected your request. This IP, site or mobile application is not authorized to use this API key.

Testing instructions (After the fix):

  • Go to Google API page and enable HTTP referrer restrictions to your site URL
  • Go to widgets page and add Contact Info & Map widget by Jetpack
  • Enter Google's API key and save the widget
  • It works as expected (displays a preview, no error).

Proposed changelog entry for your changes:

  • Fixes Google Maps referrer restrictions issue for "Contact Info & Map" widget.

@wigglemuff wigglemuff added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Extra Sidebar Widgets [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 5, 2019
@wigglemuff wigglemuff requested a review from a team as a code owner October 5, 2019 02:33
@wigglemuff wigglemuff self-assigned this Oct 5, 2019
@jetpackbot
Copy link

jetpackbot commented Oct 5, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against 9524dce

@jeherve jeherve added this to the 7.9 milestone Oct 7, 2019
jeherve
jeherve requested changes Oct 7, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fd3c20d I fixed all PHPCS warnings in that file, including spacing and a trailing comma in the changes you've introduced.

This was a minor change though. Other than that, this seems to work well. 👍 I only have one minor remark.

I'll let @kbrown9 jump in and review as well. 👍

modules/widgets/contact-info.php Outdated Show resolved Hide resolved
jeherve
jeherve reviewed Oct 7, 2019
View reviewed changes
modules/widgets/contact-info.php Outdated
Comment on lines 444 to 448
$wp_remote_get_args = array(
'headers' => array( 'Referer' => site_url() ),
);
$response = wp_remote_get( esc_url_raw( $path ), $wp_remote_get_args );

Copy link
Member

@jeherve jeherve Oct 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual changes are here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jer! :)

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 7, 2019
@mdawaffe
Copy link
Member

mdawaffe commented Oct 7, 2019

@kbrown9 (you asked about referrers elsewhere). I looked a bit more, and the URL-as-Referer logic has been there since the beginning of the Requests library we use (since WordPress 4.6 in 2016):

WordPress/Requests@78d4f3c#diff-52f160e1af3ff131e027808a6c511447R50

Apparently it was taken from the previous library, SimplePie, which has included this code since 2006 (see the get_file() method further down the page there) as a means to get around anti-hotlinking.

https://github.com/simplepie/simplepie/blob/1.0_b1/simplepie.inc#L1406-L1410

So all HTTP requests sent by WordPress set the destination URL as the Referrer because SimplePie needed a way to bypass hotlinking prevention for some image requests thirteen years ago :)

Some related issues:

kbrown9
kbrown9 previously approved these changes Oct 7, 2019
View reviewed changes
Copy link
Member

@kbrown9 kbrown9 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! I tested the branch using an API key with an HTTP restriction, and it works as expected.

Thanks for fixing this @codestor4!

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello codestor4! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33679-code before merging this PR. Thank you!

jeherve
jeherve requested changes Oct 11, 2019
View reviewed changes
modules/widgets/contact-info.php Outdated Show resolved Hide resolved
@wigglemuff @jeherve
changed site_url function to home_url
9524dce 
Co-Authored-By: Jeremy Herve <jeremy@jeremy.hu>
@wigglemuff
Copy link
Contributor Author

wigglemuff commented Oct 11, 2019 •
edited
Loading

Thanks everyone for reviewing this PR and sharing your thoughts. And thanks Jer for suggesting that commit. Sorry I am late to the party, was too occupied this week.

I think this should be good to go :)
Changing status now..

@wigglemuff wigglemuff added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Oct 11, 2019
jeherve
jeherve approved these changes Oct 14, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Merging.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 14, 2019
@jeherve jeherve merged commit 6aefc26 into master Oct 14, 2019
@jeherve jeherve deleted the fix/maps-referer branch October 14, 2019 08:44
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 14, 2019
@jeherve jeherve mentioned this pull request Oct 14, 2019
Closed
jeherve added a commit that referenced this pull request Oct 23, 2019
@jeherve
Changelog: add #13664
abdaedb
jeherve added a commit that referenced this pull request Oct 29, 2019
@jeherve
7.9: Changelog (#13608)
6cd1621 
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

@kbrown9 kbrown9 kbrown9 left review comments

Assignees

@wigglemuff wigglemuff

Labels
[Feature] Extra Sidebar Widgets Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
7.9
Development

Successfully merging this pull request may close these issues.
6 participants
@wigglemuff @jetpackbot @mdawaffe @matticbot @jeherve @kbrown9